Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for major/minor protocol version routing #443

Merged
merged 45 commits into from
Apr 17, 2020
Merged

Add support for major/minor protocol version routing #443

merged 45 commits into from
Apr 17, 2020

Conversation

nrempel
Copy link
Contributor

@nrempel nrempel commented Apr 2, 2020 •
edited
Loading

Addresses: #409, hyperledger/aries-rfcs#407

This pull requests major/minor version routing for aries protocols.

The implementation follows these rules:

  • Existing protocols (internal and external) should still work. This work is designed to not introduce any breaking changes.
  • If a protocol uses the old style, it can't receive other minor version messages. For them, the @type parameter must match exactly
  • New protocols must include a top level definition module and include references to protocol implementation as children.
  • There can only be any number of major version implementations per protocol. Each major version protocol is effectively treated as a separate protocol and is referenced by the definition module.
  • There can only be one minor version implementation per protocol major version.
  • Minimum minor version can be specified in definition.py. If a message is received for a protocol and major version with a minor version less than the minimum supported version, the agent will respond with a problem report with text describing the violation.
  • Any extra fields are now ignored when deserializing any agent messages. This ensures that protocols can support minor versions greater than the current implementation.

Remaining todo:

  • Documentation page on creating a new protocol
  • Even more manual testing?

Docs: https://github.com/nrempel/aries-cloudagent-python/blob/proto-versions/aries_cloudagent/protocols/README.md

nrempel added 30 commits March 19, 2020 09:22
@nrempel
Add debug log for loaded module
afd177f 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
refactor action menu into version subdir
7376110 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Include current minor version and validate
9da907f 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
use absolute import paths
be6e506 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Add tests and more validation restrictions
ece7149 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
more tests
ed0b676 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
more tests
c16b301 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Refactor basicmessage
efdbb06 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
update message_types path
86273d1 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Refactor connection protocol
7cb06ea 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Refactor references to connections
aac4936 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
reformat code
6287168 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Refactoring connections pacakge
8bf446e 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Refactor credentials protocol into versioned package
e1cee1a 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Refactor discovery protocol into version
9b016ed 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
fix line length
b24a1f6 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Refactor introduction protocol into pacakge
2b3a750 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Update register_routes to handle new format
136e77a 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Merge branch 'master' of https://github.com/hyperledger/aries-cloudag…
64b133c 
…ent-python into proto-versions

Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Refactor credentials protocol into versioned package
f51a7ec 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Refactor problem report into versioned pacakge
93b4686 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Refactor routing plugin into versioned package
50df870 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Refactor trustping into versioned package
eb9cadb 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Support loading admin route only packages
f20cf82 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Load each protocol version as separate plugin
065692a 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Merge branch 'master' of https://github.com/hyperledger/aries-cloudag…
9d33f34 
…ent-python into proto-versions
@nrempel
Update test
b2e4c5a 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Add versioned protocol registration
b08d72c 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
fix type bug in parsed message type
46c198f 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Support minor version greater than current
87e3f42 
Signed-off-by: Nicholas Rempel <[email protected]>
nrempel added 7 commits April 1, 2020 12:43
@nrempel nrempel requested a review from andrewwhitehead April 2, 2020 21:11
@codecov-io
Copy link

codecov-io commented Apr 2, 2020 •
edited
Loading

Codecov Report

Merging #443 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #443   +/-   ##
=======================================
  Coverage   93.74%   93.74%           
=======================================
  Files         235      235           
  Lines       11732    11732           
=======================================
  Hits        10998    10998           
  Misses        734      734

@lgtm-com
Copy link

lgtm-com bot commented Apr 2, 2020

This pull request introduces 2 alerts when merging 9981c42 into 109ec34 - view on LGTM.com

new alerts:

  • 1 for Nested loops with same variable
  • 1 for Nested loops with same variable reused after inner loop body

@nrempel nrempel requested a review from swcurran April 2, 2020 21:28
@swcurran
Copy link
Contributor

swcurran commented Apr 2, 2020

@nrempel - curious about this constraint "If a protocol uses the old style, it can't receive other minor version messages. For them, the @type parameter must match exactly". By old style, I assume you mean "did:sov:...". Not that I think it's a particular issue, but wondering why that was added?

@nrempel
Copy link
Contributor Author

nrempel commented Apr 2, 2020

@nrempel - curious about this constraint "If a protocol uses the old style, it can't receive other minor version messages. For them, the @type parameter must match exactly". By old style, I assume you mean "did:sov:...". Not that I think it's a particular issue, but wondering why that was added?

In this context by old style I mean the structure of the protocol directory. If it doesn't define it's supported major version and minimum minor version, it uses the current functionality of an exact match. Basically, it's for backwards compatibility since that is the current behaviour.

@nrempel
remove double use of variable
ebbc9a2 
Signed-off-by: Nicholas Rempel <[email protected]>
@nrempel
Copy link
Contributor Author

nrempel commented Apr 2, 2020

@TelegramSam do you have an opinion on these changes at all? I know you've been active in the spec discussion.

@nrempel nrempel marked this pull request as ready for review April 16, 2020 17:28
@nrempel
Copy link
Contributor Author

nrempel commented Apr 16, 2020

@andrewwhitehead I've opened up this PR

@andrewwhitehead
Copy link
Contributor

Added a couple comments but it looks good otherwise

@nrempel
Copy link
Contributor Author

nrempel commented Apr 17, 2020

@andrewwhitehead thanks, fixed

@andrewwhitehead andrewwhitehead merged commit cc7cb35 into openwallet-foundation:master Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewwhitehead andrewwhitehead andrewwhitehead approved these changes

@swcurran swcurran Awaiting requested review from swcurran

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nrempel @codecov-io @swcurran @andrewwhitehead